#2147 — fix(core): enforce strict runtime safety

Verdict: LGTM 🟢

The meatiest PR in the stack. Three themes:

1. noUncheckedIndexedAccess compliance. Array-index guards throughout init.ts, captionOverrides.ts, applyVariableBindings.ts. Each is the minimal correct fix — guard + early return, not defensive over-wrapping. The scheme[1]?.toLowerCase() → if (!proto) return false in isSafeMediaUrl is the most load-bearing: without the guard, a pathological URL with an empty-capture regex match would skip the allowlist check entirely (not just return undefined — it would fall through to return false, which is coincidentally correct, but the explicit guard makes the intent clear and survives future edits).

2. shouldAttemptPeriodicTimelineBind extraction. The inline transportTickCount % 60 === 0 + shouldHoldRebind logic in init.ts is pulled into a pure function in timelineRebindPolicy.ts with exported constants. This is the right level of extraction — the function is tested independently (timelineRebindPolicy.test.ts, 3 cases: periodic boundary, early-play hold, no-timeline-yet) and the behavioral tests REPLACE the regex source-shape guards in lint-runtime-preview-guards.ts (3 guards removed: usable_timeline_gate, loop_guard_rebind, early_play_rebind_hold). Good SSOT trade: behavioral coverage > source-shape regex.

The new init.test.ts case "keeps a usable bound timeline when the registry entry is replaced" is the behavioral proof that the usable-timeline gate works — it replaces window.__timelines.root after init, ticks 60 frames, and verifies the original duration holds. This closes the coverage gap left by removing the regex guard.

3. Type strictness. ComputedEffectTiming instead of { endTime?: number | string } in CSS/WAAPI adapters. this: Element on wrappedAnimate. HTMLElement narrowing in compositionLoader.ts. RuntimeTimelineChildLike and getChildren?/progress? additions to types match the GSAP API surface. (window as unknown as Record<string, unknown>) is the correct double-assertion (TypeScript won't narrow Window & typeof globalThis to Record directly).

4. Modern iteration. for...of entries() in picker.ts and timeline.ts replaces manual for + index. Not just style — with noUncheckedIndexedAccess, raw[i] would need a guard; destructuring from entries() gives a guaranteed value.

coreRuntimeBrowser.test.ts is a great addition — integration test running the IIFE in real Puppeteer, verifying the player contract (play/pause/renderSeek/getDuration), CSS adapter seek (animation currentTime = 1000ms at seek(1)), and teardown (control bridge removed, not playing after teardown message).

No SSOT violations. No behavior changes — all existing logic is preserved, just typed more strictly and tested more thoroughly.

Review by Miga

Final direct pass at current head 4df6285. Verified the requested stack-specific behavior and no unresolved current review threads remain. Current CI lanes are green; any displayed regression failure is from a superseded run, while the latest shard set passes. Approved on code merits; retain stack order.

Reviewed at 4df6285d.

Real strict-safety compliance work. The tsconfig.runtime.json + explicit typecheck:runtime gate closes a genuine hole — before this, the esbuild runtime bundle stripped types silently, so 41 real strict errors could ride into production without CI ever seeing them. Every fix I sampled is addressing an actual constraint, not defensive:

• noUncheckedIndexedAccess compliance is threaded through carefully — sole guard at init.ts:779, firstPromise guard at init.ts:2001, child.vars narrowing at init.ts:2519, colorTweens[0]? at captionOverrides.ts:138, scheme[1]?.toLowerCase() at applyVariableBindings.ts:63. Not one drops silently — each undefined branch returns a semantically correct value.
• ComputedEffectTiming | null at adapters/css.ts:40 and adapters/waapi.ts:129 replaces the hand-rolled { endTime?: number | string } structural type with the real DOM lib type. Same shape, real safety.
• function (this: Element, ...) at adapters/waapi.ts:103 — the wrapped Element.prototype.animate needs an explicit this param since apply(this, args) requires the type to flow. Correct fix.
• candidate instanceof HTMLElement at compositionLoader.ts:378 — narrows the Element union to HTMLElement at the check site, matching the local let innerRoot: HTMLElement | null declaration.
• (window as unknown as Record<string, unknown>) double-cast at init.ts:190, 1228 — the tighter window type on the base didn't permit direct string-index writes; the double-cast is the standard escape hatch here.

The regex-guard → behavior-test migration is the piece worth calling out. Three lint-runtime-preview-guards.ts guards removed:

• usable_timeline_gate → covered by the new init.test.ts:1759 behavior test (keeps a usable bound timeline when the registry entry is replaced), which swaps the window.__timelines.root entry mid-flight and asserts getDuration() still reflects the ORIGINAL bound timeline's duration. That's the exact skip-rebind-when-usable behavior the regex was guarding.
• early_play_rebind_hold → extracted into timelineRebindPolicy.ts:shouldAttemptPeriodicTimelineBind as a pure function with 3 real unit tests at timelineRebindPolicy.test.ts:9. The PLAY_REBIND_HOLD_SECONDS = 2 constant is now the single source of truth (was inlined in init.ts:298 before) — good centralization.
• loop_guard_rebind — this one's the loose thread. rebindTimelineFromResolution still exists at init.ts:1507 with the "loop_guard" | "manual" reason type, and the diagnostic code "timeline_loop_guard_rebind" still emits at :1535. But the SPECIFIC if (rebindTimelineFromResolution(resolution, "loop_guard")) call site the regex was checking for is gone from this PR's post-image (I only see the "manual" invocation at :1582). Where does "loop_guard" get invoked now? If it's called somewhere I missed, great — worth a pointer in the PR body. If the call-site was accidentally dropped when the regex-guard was removed, the loop-guard rebinding path silently dies and no test catches it (there's no equivalent to the usable_timeline_gate behavior test for this one).

Other small things:

• test-hyperframe-linter.ts no longer tests the checked-in src/tests/chat-project-9/index.html fixture — the diff at :11 replaces the fixture read with the inline VALID_COMPOSITION string. Cleaner, but if chat-project-9 had real-world edge cases (embedded scripts, atypical dimensions, unusual attribute orderings) that the inline doesn't hit, we lose that coverage. Worth confirming the fixture is either still exercised elsewhere or was intentionally retired.
• All lintHyperframeHtml callers await it. I checked studioServer.ts:356, check-hyperframe-static.ts:57, staticGuard.ts:19, browser.test.ts:12, and every test-file call site — all use await. Docs drift though: packages/core/README.md:64-66 still shows const result = lintHyperframeHtml(htmlString); (sync usage) and packages/core/docs/common-mistakes.md:69 says "Run the linter: lintHyperframeHtml(html)" without an await. Cosmetic, worth a follow-up commit or docs sweep.
• vitest.config.ts:15 — src/runtime/init.ts removed from coverage.exclude. Good; that file should be covered. If the thresholds were sized around excluding init.ts, watch that the incremental coverage requirement doesn't now push some marginal test into "required" territory.
• test:hyperframe-runtime-ci is now 9 sequential scripts. Long serial chain; if wall-clock CI becomes a concern later, some of these (test:hyperframe-runtime-behavior/-seek/-parity/-security/-duration-guards) are independent and could parallelize via a matrix.

Not blockers, but the loop_guard_rebind question is the one I'd want an answer on.

LGTM once loop_guard's new call-site is confirmed.

— Review by Rames D Jusso

Follow-up blocker from final stack audit: the strict-runtime refactor removed the loop_guard_rebind source-shape guard, but current head still has no call site for rebindTimelineFromResolution(resolution, "loop_guard") — only the "manual" invocation remains (init.ts:1582). The helper/reason/diagnostic symbols survive, so loop-guard rebinding appears silently regressed. Please restore/cover the loop-guard path (and update behavior tests) before re-review. Existing approval is superseded by this finding.

Verdict: 🔴 Concurring with Miguel's blocker at 4df6285

Independent read went through my own adversarial pass and reached 🟢 LGTM (traced the timeline-rebind extraction against parent b24c7bb, verified the window cast cluster, refuted the zero-adapter short-circuit concern). Miguel's CHANGES_REQUESTED (16:53:52Z) posted while my agent was drafting — his read supersedes mine and is correct on the load-bearing point. Documenting the miss + adding evidence.

Miguel's blocker — verified independently

Read packages/core/src/runtime/init.ts at head 4df6285:

• Line 1509: reason: "loop_guard" | "manual", — helper signature accepts both.
• Line 1535: code: "timeline_loop_guard_rebind" — diagnostic code for the loop-guard branch.
• Line 1582: if (rebindTimelineFromResolution(resolution, "manual")) { … } — ONLY call site; no "loop_guard" invocation exists.

The "loop_guard" branch has full machinery (type param, diagnostic code, helper implementation) but zero callers. Miguel is right: the deleted loop_guard_rebind regex-guard in packages/core/src/lint-runtime-preview-guards.ts was documenting an unimplemented (or silently regressed) feature, not lint decoration. Removing the guard makes the regression harder to catch. Fix path is either (a) restore the "loop_guard" call site with correct trigger conditions + behavior test, or (b) explicitly remove the type union + diagnostic code + helper for the "loop_guard" reason to formalize the regression as intentional.

My other findings — LGTM-independent from the blocker

F1. Timeline rebind extraction — behavior-preserving. shouldAttemptPeriodicTimelineBind in packages/core/src/runtime/timelineRebindPolicy.ts:5-22 is a byte-equivalent predicate to the legacy inline gate at init.ts:2632-2637 (parent b24c7bb). Legacy: if (transportTickCount % 60 === 0) { const shouldHoldRebind = playing && haveTimeline && now<2; if (!shouldHoldRebind) {…} }. New: A && !(B && C && D) in both. tick <= 0 and Number.isInteger guards in the new function are defensive-unreachable (transportTickCount starts at 0 and += 1 runs on line 2633 before the gate call at 2637, so the smallest observed tick is 1). Snapshots at the call site match legacy expressions — no stale-ref drift.

F2. (window as unknown as Record<string, unknown>) cast cluster — pacifier only. Two write-side casts changed shape (init.ts:190 for __timelines, init.ts:1228 for __hfStudioManualEditsApply), both single-hop → double-hop for strict mode. Typed shape at window.d.ts:31 unchanged; all 20+ readers still coerce via (window.__timelines ?? {}) as Record<string, RuntimeTimelineLike | undefined>. No consumer required a matching update because the write target property name is unchanged.

F3. Zero-adapter short-circuit — recon premise refuted. Legacy already had explicit promises.length === 0 → return true at init.ts:1990-1993 (parent b24c7bb), which runs BEFORE any Promise.all construction. New code retains that same length-0 return at init.ts:1992-1995. The subsequent if (!firstPromise) return true; at 2001-2002 is strict noUncheckedIndexedAccess pacifier — behaviorally unreachable because promises.length >= 1 is already guaranteed. Zero-adapter callers get identical trackedAdapterReadyPromise = null, trackedAdapterReadySettled = true, return true behavior.

F4. Producer integration test is real, correctly gated. coreRuntimeBrowser.test.ts loads packages/core/dist/hyperframe.runtime.iife.js in headless puppeteer, awaits runtime-set __playerReady && __renderReady, asserts real seek via getAnimations()[0].currentTime. Not aspirational. Added to INTEGRATION_TEST_FILES in test-classification.mjs:12 per the base PR's lane classifier. Correctly gated.

F5. Other deleted regex-guards. usable_timeline_gate, early_play_rebind_hold, and shouldHoldRebindDuringEarlyPlay had regexes that already did not match legacy source at b24c7bb (checked each pattern against the parent's init.ts). Deleting those three is safe; unlike loop_guard_rebind, they had no surviving helper/diagnostic/type-union machinery signaling an unimplemented feature. The four kept guards (external_compositions_gate, child_timeline_activation, root_unusable_fallback, external_script_ordering, external_script_load_wait) all match their referenced source patterns at head.

Adversarial-pass reflection (what my initial LGTM missed)

Ran the required adversarial pass and reached 🟢 LGTM. Miguel's find shows the pass was scoped too narrowly: I checked each deleted guard against "does the regex match the legacy source" and concluded "dead lint" when the answer was no. The load-bearing question — "does the guard's target feature exist and reach its guarded branch?" — is a separate check I didn't run. Same lens as feedback_verify_new_path_reachable.md in my memory but extended: not just "does the fix reach its cohort" but "does the guard's implied feature reach a caller." Two convergent LGTMs (Miga + Rames-D) and Miguel's initial APPROVED made the miss cheap to fall into. Saving this lesson.

R1 by Via (concurring with Miguel's CHANGES_REQUESTED)

R2 update — verifying James's rebuttal, verdict revised to 🟢 LGTM at 5634cdf

James asked Vai and me to re-review based on his rebuttal to the loop_guard blocker. Ran the parent-vs-head verification independently — his mechanic is correct. My R1 concurrence with Magi's CHANGES_REQUESTED was misdirected against this specific PR.

Independent verification

packages/core/src/runtime/init.ts at merged parent main@077d7e17:

1505:  const rebindTimelineFromResolution = (
1507:    reason: "loop_guard" | "manual",
1533:      code: "timeline_loop_guard_rebind",
1580:      if (rebindTimelineFromResolution(resolution, "manual")) {

packages/core/src/runtime/init.ts at #2147 head 5634cdf2:

1507:  const rebindTimelineFromResolution = (
1509:    reason: "loop_guard" | "manual",
1535:      code: "timeline_loop_guard_rebind",
1582:      if (rebindTimelineFromResolution(resolution, "manual")) {

Both have: type union "loop_guard" | "manual", diagnostic code "timeline_loop_guard_rebind", helper implementation, and only the "manual" call site. Zero "loop_guard" invocations at either SHA. Delta from parent to head is line-shift only — none of the touched hunks in this PR add or remove a "loop_guard" call site.

James's read holds: the "loop_guard" reason branch is pre-existing dead contract on main, not a behavior regression introduced by #2147. The only change in this PR to that surface is removing the corresponding stale entry in packages/core/scripts/lint-runtime-preview-guards.ts — a regex that never matched the parent's source either.

Refined verdict

• The lint-pattern removal is safe cleanup of documentation for pre-existing dead code. Not a regression cause.
• The surviving "loop_guard" reason / diagnostic / type-union machinery is dead contract as James notes, and is worth a follow-up (either formalize the deprecation by dropping the union member + diagnostic + helper branch, or restore the intended call site with behavior tests). Not blocking on #2147.
• Restoring "an unverified call site" here — as James notes — would introduce new behavior rather than preserve the parent contract. That's the correct scope call.

R1 correction

My R1 concurred with Magi's CHANGES_REQUESTED on the reasoning "surviving helper/diagnostic without a call site = regressed feature signalled by the removed guard." The reasoning was sound as a general discipline but its application to this PR was misdirected — I verified feature reachability at the head SHA (which showed the missing call site) without verifying the same at the parent SHA (which would have shown the call site was already missing). Had I run the parent-vs-head diff on the feature's call sites specifically, I would have caught that #2147 is not the cause.

The general lesson (verify feature reachability end-to-end when a guard is deleted) still holds. The refinement: run the reachability check at BOTH parent and head — if the feature is already unreachable at parent, the guard's deletion is cleanup, not regression-introduction.

Verdict revised: 🟢 LGTM at 5634cdf on the mechanic. Suggesting Magi + Miga review James's rebuttal + this parent-vs-head evidence and dismiss the change request if the shared read agrees. The pre-existing "loop_guard" dead contract is worth a separate cleanup PR either way.

R2 by Via (verifying and concurring with jrusso1020's rebuttal)

Re-reviewed current head 5634cdf. The loop_guard contract is pre-existing dead code on main; this PR only removes a zero-match lint pattern and introduces no regression. Approving the current head.

Post-merge advisory review (this PR is already merged into main). Findings flagged for follow-up, not gating.

Reviewed the final head against its recorded parent ebbd1eb2, not cumulatively against the stack root. No new findings.

• packages/core/src/runtime/timelineRebindPolicy.ts:10-21 is behavior-equivalent to the prior positive-integer tick / 60-frame / first-two-seconds hold gate, with focused boundary tests.
• packages/producer/src/services/coreRuntimeBrowser.test.ts:12-36,38-92 exercises the built IIFE in Chrome, proves CSS seek and public player behavior, and closes the browser in afterAll while verifying teardown removes the control bridge.
• I independently repeated the parent/head reachability check behind the earlier loop_guard discussion: both parent and head have only the pre-existing "manual" call site. This PR removes a zero-match source-shape regex; it does not remove runtime behavior. The dead loop_guard contract remains a separate cleanup opportunity, as already documented.

Verification: 113 focused core tests passed; runtime typecheck, five remaining source guards, and linter integration passed; the real-browser producer integration set passed 12/12. All 56 exact final-head GitHub checks completed without failure.

Verdict: COMMENT (post-merge: Ready)
Reasoning: Strict-runtime fixes preserve behavior, the behavioral/browser coverage is real, and the only disputed guard was confirmed pre-existing dead contract rather than a regression from this PR.

— Deepwork